Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

nixos/nextcloud: add an extraPackages option #371084

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

nixos/nextcloud: add an extraPackages option #371084

wants to merge 1 commit into from

Conversation

balsoft
Copy link
Member

@balsoft balsoft commented Jan 5, 2025 •
edited
Loading

Adds an extraPackages option to easily add packages to nextcloud's PATH.

This is useful to enable video thumbnail generation by adding ffmpeg there.

I feel like we could also add ffmpeg to extraPackages by default as it's quite useful, but maybe there's a reason not to do this.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 25.05 Release Notes (or backporting 24.11 and 25.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@balsoft balsoft requested a review from Ma27 January 5, 2025 08:44
@github-actions github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` labels Jan 5, 2025
@Shawn8901
Copy link
Contributor

Fyi @SuperSandro2000 : as the nuschtos module for nextcloud does similar things, your might be interested in that

Ma27
Ma27 reviewed Jan 5, 2025
View reviewed changes
Copy link
Member

@Ma27 Ma27 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do add everything from environment.systemPackages here already.
I'm a little hesitant to change that because of the potential fallout for people, but I'd very much prefer to have one of extraPackages or systemPackages for that.

Ma27
Ma27 requested changes Jan 5, 2025
View reviewed changes
Copy link
Member

@Ma27 Ma27 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, the manual build fails.

@drupol
Copy link
Contributor

drupol commented Jan 5, 2025

The manual fails also in my PRs (#370895 (comment)). I don't think the issue is related to this PR.

@Ma27
Copy link
Member

Ma27 commented Jan 5, 2025 

       … from call site
         at /home/runner/work/nixpkgs/nixpkgs/nixos/modules/services/web-apps/nextcloud.nix:326:21:
          325|
          326|     extraPackages = mkOption {
             |                     ^
          327|       type = types.listOf types.package;

       error: function 'mkOption' called with unexpected argument 'exampleText'
       at /home/runner/work/nixpkgs/nixpkgs/lib/options.nix:67:5:
           66|   mkOption =
           67|     {
             |     ^
           68|     # Default value used when no definition is given in the configuration.

This looks very much like it's related to this PR.

@wegank wegank added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Jan 5, 2025
@drupol
Copy link
Contributor

drupol commented Jan 5, 2025

Oh indeed!!! Oops :s

@balsoft
nixos/nextcloud: add an extraPackages option
7cd6171 
Adds an extraPackages option to easily add packages to nextcloud's PATH.
@balsoft balsoft force-pushed the nextcloud-extraPackages branch from b064eb5 to 7cd6171 Compare January 6, 2025 07:42
@balsoft
Copy link
Member Author

balsoft commented Jan 6, 2025 •
edited
Loading

We do add everything from environment.systemPackages here already.

Right, but that seems to be a very non-NixOS-y way of doing things. I don't want to have ffmpeg installed on my system ( definitely not by default when installing nextcloud) and yet it would be quite useful to have ffmpeg in nextcloud's PATH.

As for backwards-compatibility, maybe we can warn about for a release or two and then actually switch to extraPackages.

@github-actions github-actions bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 labels Jan 6, 2025
@wegank wegank removed the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Jan 6, 2025
@balsoft balsoft requested a review from Ma27 January 27, 2025 07:09
Ma27
Ma27 reviewed Feb 5, 2025
View reviewed changes
nixos/modules/services/web-apps/nextcloud.nix
@@ -323,6 +323,15 @@ in {
'';
};

extraPackages = mkOption {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the sake of consistency, shouldn't these be added to nextcloud-cron & nextcloud-update-db?
Especially the former may also need things that one would add here.

nixos/modules/services/web-apps/nextcloud.nix
"/run/current-system/sw"
"/usr"
"" # adds "/bin" to PATH
]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I meant with

but I'd very much prefer to have one of extraPackages or systemPackages for that.

was to either use these paths xor extraPackages for everything.

Given we have an agreement to go for extraPackages, I'd suggest:

  • we add a warning to the release notes about the removal of these paths in 25.11 (to the 25.05 release notes)
  • remove this section again
  • and use extraPackages everywhere, as mentioned above.

@Ma27 Ma27 mentioned this pull request Feb 11, 2025
Open
13 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Ma27 Ma27 Ma27 requested changes

@drupol drupol drupol approved these changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@balsoft @Shawn8901 @drupol @Ma27 @wegank